Quill.js delta payload for arrayNode events#26677
Quill.js delta payload for arrayNode events#26677daesunp wants to merge 3 commits intomicrosoft:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Quill-style delta payload for array-node nodeChanged events by plumbing sequence-field mark data through the delta visitation/eventing pipeline and exposing it as ArrayNodeDeltaOp[] on NodeChangedData.
Changes:
- Introduces
ArrayNodeDeltaOpand addsNodeChangedData.deltafor array nodes, updatingTreeChangeEventsBeta.nodeChangedtyping accordingly. - Extends internal delta visitation (
DeltaVisitor.fieldMarks) and anchor/kernel events (childrenChangedAfterBatch.fieldMarks) to carry ordered field mark sequences through buffering. - Adds/updates unit tests and introduces a new fuzz suite to validate array delta behavior.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/framework/fluid-framework/api-report/fluid-framework.legacy.beta.api.md | API report updates to include ArrayNodeDeltaOp and NodeChangedData.delta typing. |
| packages/framework/fluid-framework/api-report/fluid-framework.beta.api.md | Same as above for beta report. |
| packages/framework/fluid-framework/api-report/fluid-framework.alpha.api.md | Same as above for alpha report. |
| packages/dds/tree/src/test/simple-tree/core/treeNodeKernel.spec.ts | Adds tests for array nodeChanged delta presence/semantics and move delta payloads. |
| packages/dds/tree/src/test/simple-tree/api/treeNodeApi.spec.ts | Adds API-level tests validating concrete ArrayNodeDeltaOp sequences for array operations. |
| packages/dds/tree/src/test/shared-tree/fuzz/arrayNodeDelta.fuzz.spec.ts | New fuzz suite intended to validate delta-driven shadow maintenance across clients/rebase. |
| packages/dds/tree/src/simple-tree/index.ts | Re-exports ArrayNodeDeltaOp from the simple-tree entrypoint. |
| packages/dds/tree/src/simple-tree/core/unhydratedFlexTree.ts | Ensures unhydrated childrenChangedAfterBatch emits an (empty) fieldMarks map. |
| packages/dds/tree/src/simple-tree/core/treeNodeKernel.ts | Buffers/flushes fieldMarks alongside changedFields, invalidating marks on multi-batch collisions. |
| packages/dds/tree/src/simple-tree/api/treeNodeApi.ts | Defines ArrayNodeDeltaOp, converts DeltaMark[] → ops, and emits delta for array nodeChanged. |
| packages/dds/tree/src/simple-tree/api/treeBeta.ts | Updates NodeChangedData/TreeChangeEventsBeta typings + docs to surface array delta. |
| packages/dds/tree/src/simple-tree/api/index.ts | Re-exports ArrayNodeDeltaOp from the API barrel. |
| packages/dds/tree/src/index.ts | Re-exports ArrayNodeDeltaOp from the package root. |
| packages/dds/tree/src/core/tree/visitorUtils.ts | Extends combined visitors to forward the new optional fieldMarks hook. |
| packages/dds/tree/src/core/tree/visitDelta.ts | Adds DeltaVisitor.fieldMarks and calls it once per field during the detach pass. |
| packages/dds/tree/src/core/tree/anchorSet.ts | Extends childrenChangedAfterBatch payload to include fieldMarks captured from delta visitation. |
| packages/dds/tree/api-report/tree.legacy.beta.api.md | API report updates for new type/export and event payload typing. |
| packages/dds/tree/api-report/tree.beta.api.md | Same as above for beta report. |
| packages/dds/tree/api-report/tree.alpha.api.md | Same as above for alpha report. |
You can also share your feedback on Copilot code review. Take the survey.
| } | ||
| | { readonly type: "synchronize" }; |
| for (const op of delta) { | ||
| switch (op.type) { | ||
| case "retain": { | ||
| result.push(...shadow.slice(srcIdx, srcIdx + op.count)); | ||
| srcIdx += op.count; | ||
| break; |
|
🔗 No broken links found! ✅ Your attention to detail is admirable. linkcheck output |
| @@ -372,6 +392,9 @@ function visitFieldMarks( | |||
| if (fields !== undefined) { | |||
| for (const [key, field] of fields) { | |||
| visitor.enterField(key); | |||
| if (config.isFirstPass) { | |||
| visitor.fieldMarks?.(key, field.marks); | |||
There was a problem hiding this comment.
Instead of adding the isFirstPass flag, can we just put this line into the beginning of the attachPass function?
| // Replacement: content removed and new content attached in the same position. | ||
| // The `Delta.Mark` format allows both to be set simultaneously (e.g. when a slot's | ||
| // content is atomically swapped), even though the sequence-field encoder does not | ||
| // currently emit such marks for array (EmptyKey) fields. Handle it defensively. |
There was a problem hiding this comment.
The terminology here doesn't make sense to me. It seems sufficient to say that this case should be unreachable (because sequence field never produces a replace delta mark), but is handled anyway.
| if (mark.attach !== undefined && mark.detach !== undefined) { | ||
| // Replacement: content removed and new content attached in the same position. | ||
| // The `Delta.Mark` format allows both to be set simultaneously (e.g. when a slot's | ||
| // content is atomically swapped), even though the sequence-field encoder does not | ||
| // currently emit such marks for array (EmptyKey) fields. Handle it defensively. | ||
| ops.push({ type: "remove", count: mark.count }); | ||
| ops.push({ type: "insert", count: mark.count }); | ||
| } else if (mark.attach !== undefined) { | ||
| ops.push({ type: "insert", count: mark.count }); | ||
| } else if (mark.detach === undefined) { | ||
| // Neither attach nor detach: elements retained (may have nested changes in mark.fields). | ||
| ops.push({ type: "retain", count: mark.count }); | ||
| } else { | ||
| ops.push({ type: "remove", count: mark.count }); | ||
| } |
There was a problem hiding this comment.
It looks like this could just have one block for mark.detach !== undefined, followed by a block for mark.attach !== undefined.
| arg?: { | ||
| changedFields: ReadonlySet<FieldKey>; | ||
| }, | ||
| ...args: |
There was a problem hiding this comment.
What is the purpose of this change?
Description
Adds quill.js delta event payload for arraynodes.